Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add possibility to run sidecars with vault #87

Merged
merged 8 commits into from
Nov 21, 2019

Conversation

shahbazn
Copy link
Contributor

Problem: This chart does not allow running sidecar containers with vault, that might me needed for some scenarios e.g. running prometheus exporter.

Solution: Add configurable sidecar containers to the chart

@hashicorp-cla
Copy link

hashicorp-cla commented Oct 17, 2019

CLA assistant check
All committers have signed the CLA.

@ncabatoff
Copy link

You shouldn't need a prometheus exporter now that prometheus metrics can be scraped directly from Vault.

@shahbazn
Copy link
Contributor Author

@ncabatoff agreed running an exporter is not necessary now that the metrics api can be scrapped directly, but might still be enough for some users to run an exporter which only polls the health api and already works with existing grafana dashboards.

Could there be other cases of running auxiliary services with vault?

@ncabatoff
Copy link

Could there be other cases of running auxiliary services with vault?

Sure, I wasn't suggesting the PR should be rejected, I was just letting you know in case you weren't aware that a prometheus exporter isn't strictly necessary.

@jasonodonnell
Copy link
Contributor

jasonodonnell commented Oct 18, 2019

Hi @shahbazn, you'll need to sign the CLA for us to consider this PR. Additionally can you add unit tests for this configurable? You can find the existing tests under test/unit. Thanks!

@shahbazn
Copy link
Contributor Author

Hi @jasonodonnell I have added the unit tests now. Had to force push the initial commits to include my email address for CLA to work.

@shahbazn
Copy link
Contributor Author

Hi @jasonodonnell the PR is ready to be reviewed.

@AnthonyHerman
Copy link

FWIW I'm using this already. Works great.

@jasonodonnell jasonodonnell self-requested a review November 4, 2019 14:47
Copy link
Contributor

@jasonodonnell jasonodonnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@jasonodonnell
Copy link
Contributor

Hi @shahbazn, upon further review there's an error in your test code. You are missing a closing brace on server/standalone-StatefulSet: no extra containers added. This will need to be fixed prior to merging.

@iusergii
Copy link
Contributor

This can be solution for #109.

Copy link
Contributor

@jasonodonnell jasonodonnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @shahbazn !

@jasonodonnell jasonodonnell merged commit faf5a84 into hashicorp:master Nov 21, 2019
radudd pushed a commit to radudd/vault-helm that referenced this pull request Jun 5, 2020
* Add extra containers

* fix template

* add unit tests

* resolve conflicts

* remove duplicate docs

* fix unit tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants